Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Release list fixes & broader use #393

Merged
merged 3 commits into from
Dec 17, 2022
Merged

Release list fixes & broader use #393

merged 3 commits into from
Dec 17, 2022

Conversation

zenhack
Copy link
Contributor

@zenhack zenhack commented Dec 16, 2022

This patch fixes a bug I caught in releaseList: because .Release() is defined on a non-pointer receiver, doing defer rl.Release() dereferences rl up front, so future calls to .Add() don't actaully affect the slice that gets passed to .Release(). So this patch makes the receiver a pointer to avoid this.

It also expands the use of defer for releaseLists (the bug was caught in the process of doing so).

Finally, I made a simplification to the locking in handleFinish, to use defer for the unlock. Maybe this should have been its own PR, but... oh well.

@zenhack
releaseList: take a pointer receiver for .Release()
3a31c93 
Otherwise, defer rl.Release() dereferences rl eagerly, such that if
items are added to *rl after the defer, they are not actually invoked.
@zenhack
Clean up locking & releaseList use in handleFinish
5a6941d
@zenhack
Simplify releaseList use in answer.Return and handleCall
0358e8f
@lthibault
Copy link
Collaborator 

=== RUN   TestRecvDisembargo
Error:     level0_test.go:2124: conn error: rpc: incoming disembargo: answer ID 55 has not sent return
Error:     level0_test.go:2124: conn error: incoming return: send finish: build message: rpc: connection closed
Error:     level0_test.go:2124: conn error: send return: rpc: connection closed
Error:     level0_test.go:2124: conn error: send return: rpc: connection closed
Error:     level1_test.go:767: don't know how to handle abort; skipping
Error:     level1_test.go:674: recvMessage(ctx, p2): transport: stream transport: receive: io: read/write on closed pipe
--- FAIL: TestRecvDisembargo (0.09s)

I think this is a new one.

@lthibault
Copy link
Collaborator

On second thought, this looks like #377.

@lthibault
Copy link
Collaborator 

==================
WARNING: DATA RACE
Write at 0x00c000000088 by goroutine 4399:
  capnproto.org/go/capnp/v3.alloc()
      /home/runner/work/go-capnproto2/go-capnproto2/message.go:356 +0x1d2
  capnproto.org/go/capnp/v3.NewCompositeList()
      /home/runner/work/go-capnproto2/go-capnproto2/list.go:62 +0xe4
  capnproto.org/go/capnp/v3/std/capnp/rpc.NewCapDescriptor_List()
      /home/runner/work/go-capnproto2/go-capnproto2/std/capnp/rpc/rpc.capnp.go:2406 +0xce
  capnproto.org/go/capnp/v3/std/capnp/rpc.Payload.NewCapTable()
      /home/runner/work/go-capnproto2/go-capnproto2/std/capnp/rpc/rpc.capnp.go:2179 +0x84
  capnproto.org/go/capnp/v3/rpc.(*Conn).fillPayloadCapTable()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/export.go:183 +0x1fa
  capnproto.org/go/capnp/v3/rpc.(*answer).sendReturn()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/answer.go:228 +0x174
  capnproto.org/go/capnp/v3/rpc.(*answer).Return()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/answer.go:197 +0x11d
  capnproto.org/go/capnp/v3/server.(*Server).handleCall()
      /home/runner/work/go-capnproto2/go-capnproto2/server/server.go:214 +0x281
  capnproto.org/go/capnp/v3/server.(*Server).handleCalls.func2()
      /home/runner/work/go-capnproto2/go-capnproto2/server/server.go:182 +0x84
  capnproto.org/go/capnp/v3/server.(*Server).handleCalls()
      /home/runner/work/go-capnproto2/go-capnproto2/server/server.go:183 +0x1c4
  capnproto.org/go/capnp/v3/server.New.func1()
      /home/runner/work/go-capnproto2/go-capnproto2/server/server.go:122 +0x58

Previous read at 0x00c000000088 by goroutine 4405:
  capnproto.org/go/capnp/v3.(*Segment).slice()
      /home/runner/work/go-capnproto2/go-capnproto2/segment.go:49 +0x5d
  capnproto.org/go/capnp/v3.(*Segment).readUint64()
      /home/runner/work/go-capnproto2/go-capnproto2/segment.go:65 +0x51
  capnproto.org/go/capnp/v3.(*Segment).readRawPointer()
      /home/runner/work/go-capnproto2/go-capnproto2/segment.go:69 +0x50
  capnproto.org/go/capnp/v3.(*Segment).resolveFarPointer()
      /home/runner/work/go-capnproto2/go-capnproto2/segment.go:235 +0x4f
  capnproto.org/go/capnp/v3.(*Segment).readPtr()
      /home/runner/work/go-capnproto2/go-capnproto2/segment.go:116 +0x75
  capnproto.org/go/capnp/v3.Struct.Ptr()
      /home/runner/work/go-capnproto2/go-capnproto2/struct.go:109 +0x119
  capnproto.org/go/capnp/v3.Transform()
      /home/runner/work/go-capnproto2/go-capnproto2/answer.go:580 +0x2ed
  capnproto.org/go/capnp/v3.resolution.ptr()
      /home/runner/work/go-capnproto2/go-capnproto2/answer.go:606 +0xfc
  capnproto.org/go/capnp/v3.resolution.client()
      /home/runner/work/go-capnproto2/go-capnproto2/answer.go:615 +0x96
  capnproto.org/go/capnp/v3.(*Answer).PipelineRecv()
      /home/runner/work/go-capnproto2/go-capnproto2/answer.go:393 +0x413
  capnproto.org/go/capnp/v3.(*Answer).PipelineRecv-fm()
      <autogenerated>:1 +0xc7
  capnproto.org/go/capnp/v3/server.queueCaller.PipelineRecv()
      /home/runner/work/go-capnproto2/go-capnproto2/server/answer.go:161 +0x2e2
  capnproto.org/go/capnp/v3/server.(*answerQueue).PipelineRecv()
      /home/runner/work/go-capnproto2/go-capnproto2/server/answer.go:136 +0xc9
  capnproto.org/go/capnp/v3/rpc.(*Conn).handleCall()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/rpc.go:837 +0x1f32
  capnproto.org/go/capnp/v3/rpc.(*Conn).receive()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/rpc.go:543 +0x644
  capnproto.org/go/capnp/v3/rpc.(*Conn).receive-fm()
      <autogenerated>:1 +0x39
  capnproto.org/go/capnp/v3/rpc.(*Conn).backgroundTask.func1()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/rpc.go:206 +0x92
  golang.org/x/sync/errgroup.(*Group).Go.func1()
      /home/runner/go/pkg/mod/golang.org/x/[email protected]/errgroup/errgroup.go:57 +0x91

Goroutine 4399 (running) created at:
  capnproto.org/go/capnp/v3/server.New()
      /home/runner/work/go-capnproto2/go-capnproto2/server/server.go:122 +0x544
  capnproto.org/go/capnp/v3/rpc/internal/testcapnp.CapArgsTest_NewServer()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/internal/testcapnp/test.capnp.go:654 +0xc9
  capnproto.org/go/capnp/v3/rpc/internal/testcapnp.CapArgsTest_ServerToClient()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/internal/testcapnp/test.capnp.go:660 +0x36
  capnproto.org/go/capnp/v3/rpc.TestCallReceiverAnswerRpc()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/receiveranswer_test.go:108 +0x28b
  testing.tRunner()
      /opt/hostedtoolcache/go/1.19.4/x64/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /opt/hostedtoolcache/go/1.19.4/x64/src/testing/testing.go:1493 +0x47

Goroutine 4405 (running) created at:
  golang.org/x/sync/errgroup.(*Group).Go()
      /home/runner/go/pkg/mod/golang.org/x/[email protected]/errgroup/errgroup.go:54 +0xee
  capnproto.org/go/capnp/v3/rpc.NewConn()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/rpc.go:172 +0x744
  capnproto.org/go/capnp/v3/rpc.TestCallReceiverAnswerRpc()
      /home/runner/work/go-capnproto2/go-capnproto2/rpc/receiveranswer_test.go:105 +0x2d3
  testing.tRunner()
      /opt/hostedtoolcache/go/1.19.4/x64/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /opt/hostedtoolcache/go/1.19.4/x64/src/testing/testing.go:1493 +0x47
==================

Is this one new?

@zenhack
Copy link
Contributor Author

zenhack commented Dec 16, 2022

No, that's the one that was re-intrroduced when we backed out the fix for #349

@lthibault lthibault merged commit 6b08920 into capnproto:main Dec 17, 2022
@zenhack zenhack deleted the releaseList-fixups branch December 17, 2022 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lthibault lthibault lthibault approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zenhack @lthibault